Implement advance_by, advance_back_by for iter::Chain #77594
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Sure, I'll take this one. |
There was a problem hiding this comment.
The code here all looks right to me, but I've become paranoid about early-exits and fusing in &mut self iterator methods.
Could you please add some tests for the edge cases of non-fused iterators? You're self.a = None;ing in the right places, AFAICT, but I don't think the tests right now would catch it if you weren't (since slice::Iter is always fused anyway).
If you need a non-fused iterator, you could use something like
pub fn make_silly_iterator(mut x: u8) -> impl Iterator<Item = u8> {
std::iter::from_fn(move || {
let i = x;
x = x.wrapping_sub(1);
if i == 0 { None } else { Some(i) }
})
}|
Good call, I've been meaning to add more tests with non-fused iterators across the board because the current tests are barely using any. It wouldn't surprise me if some of the iterators currently contain bugs related to that. I'm adding a |
|
The tests now indeed do fail if I leave out |
|
Thanks! Looks like a plausible helper to use in more tests ( @bors r+ |
|
📌 Commit 1d27a50 has been approved by |
|
@bors rollup=never |
|
☀️ Test successful - checks-actions, checks-azure |
|
As mentioned in #76909 (comment), this was a pretty bad regression on the deeply-nested stress test. https://perf.rust-lang.org/compare.html?start=5ded394553296d56bb66e925d7001ab3271979ce&end=5849a7eca90582ee59b67eb09548a2aa424d7f52&stat=instructions:u |
Part of #77404.
This PR does two things:
Chain::advance[_back]_byin terms ofadvance[_back]_byonself.aandadvance[_back]_byonself.bChain::nth[_back]to useadvance[_back]_byonself.aandnth[_back]onself.bThis ensures that
Chain::nthcan take advantage of an efficientnthimplementation on the second iterator, in case it doesn't implementadvance_by.cc @scottmcm in case you want to review this